Added three new messages for gridded atmospheric SSR corrections. #686
Conversation
martin-swift
requested review from
benjamin0,
benjaminaltieri,
jbangelo,
mfine,
pmiettinen,
scarcanague and
silverjam
mfine
left a comment
mfine
left a comment
There was a problem hiding this comment.
Just one comment on using GNSS Signal. Will let others comment on the content.
pmiettinen
left a comment
pmiettinen
left a comment
There was a problem hiding this comment.
I don't know is it mentioned in the instructions but it is preferred that the generated files changes are in separate commit. Also mixing formatting changes to actual change is not optimal. Messages seem ok.
benjamin0
left a comment
benjamin0
left a comment
There was a problem hiding this comment.
Looks great! Just a few comments, could you double check the spelling before merge?
| type: u8 | ||
| desc: Postion of this message in the dataset | ||
| - ssr_update_interval: | ||
| type: u16 |
There was a problem hiding this comment.
not if we want the units to be seconds. (this is one of the places I deviated from the spec)
| type: u8 | ||
| desc: Postion of this message in the dataset | ||
| - ssr_update_interval: | ||
| type: u16 |
|
A general improvement would be to include the units of the different fields in the description, that way the SBP spec is self contained and you don't need to go look up the units from the proposal docs |
martin-swift
force-pushed
the
martin/ORI-498-SSR-3GPP-iono
branch
from
d07c745 to
1fe0aeb
Compare
spec/yaml/swiftnav/sbp/ssr.yaml
Outdated
| - sv_id: | ||
| type: SvId | ||
| desc: Unique space vehicle identifier | ||
| - stec_quality_indicator: |
There was a problem hiding this comment.
So the spec tells us how to calculate "Q" from the this value, but what are the units of Q? @benjamin0
spec/yaml/swiftnav/sbp/ssr.yaml
Outdated
| desc: space vehicle identifier | ||
| - residual: | ||
| type: s16 | ||
| desc: STEC residual (Scale factor 0.04 TECU) |
There was a problem hiding this comment.
and what's "U" here? Total Electron Content...? @benjamin0
martin-swift
force-pushed
the
martin/ORI-498-SSR-3GPP-iono
branch
from
1fe0aeb to
39b4e8f
Compare
|
Everyone happy? I want to push the button on this today. |
silverjam
left a comment
silverjam
left a comment
There was a problem hiding this comment.
Can't really comment from a content perspective but the code looks good.
benjamin0
left a comment
benjamin0
left a comment
There was a problem hiding this comment.
One question about the num_msgs/seq_num fields in GridDefinitionHeader, otherwise lgtm!
| - lon_nw_corner_enc: | ||
| type: u16 | ||
| desc: encoded longitude of the northwest corner of the grid | ||
| - num_msgs: |
There was a problem hiding this comment.
Will there be more than 1 GridDefinitionHeader messages?
There was a problem hiding this comment.
that was done to support the RLE validity list. I think that thing is pretty janky. Why would we ever send invalid data? I'd be more than happy to snip that whole RLE list out of the message, and if we really need it, include validity flag in the correction/STEC messages. Thoughts?
There was a problem hiding this comment.
I think I see where they are going with this; because it's broadcast, everyone is consuming it. If the rover is in some corner case, we might want to say "hey, don't use this, because you are out of bounds from the model/whatever"
There was a problem hiding this comment.
It would be a minor deviation from spec, but I could chop out the multi message support, since I think is it unlikely we'll see an RLE list that long, and you said these aren't prod messages. yes/no?
There was a problem hiding this comment.
Yeah I forgot about the RLE validity list, let's keep it.
|
@martin-swift Travis failure is the same semi-transient error we've seen for a while, I think this is good to merge. |
Implementing messages from https://drive.google.com/file/d/1jMLC2cjvVk6TUDGuktVLeK3kPyn4yi7S/view and https://drive.google.com/file/d/1RKjSgyBlxBhFz7ipExKh_Z2gFU_Yb0gR/view
I tried to be fairly faithful to the spec, but I tried to avoid things like bit packing and I was also working with the constraints of SBP, particularly the 255 max message size and the fact you can only have one variable length array in a message.